fix: handle 1D interpolation result in resample for single-state ODEs - #1
fix: handle 1D interpolation result in resample for single-state ODEs#1yasumorishima wants to merge 4 commits into
Conversation
When an ODESystem has only a single state variable, scipy interpolation returns a 1D array (n,) instead of 2D (n, 1). This causes a broadcast error when assigning to the pre-allocated 2D xs array. Fix by checking ndim and adding a column axis when needed. Fixes nasa#70
📝 WalkthroughWalkthroughIntercepts interpolation output in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
1 similar comment
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
6 tests covering: - Basic single-state resample - Resampled values match analytical solution (exponential decay) - include_output=False path - dynamic_output with single-state ODE - Different dt values - Multi-state ODE regression (harmonic oscillator) Also discovered that include_events=True triggers a separate IndexError (issue nasa#71, t_size calculation off-by-2) — tests use include_events=False to isolate the ndim fix from that pre-existing bug. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_trajectory_analysis.py (2)
388-463: Consider DRYing repeated single-state ODE definitions.The same ODE/Trajectory scaffolding is repeated across several tests (Line 390–Line 413, Line 420–Line 428, Line 452–Line 460). A small helper would reduce noise and make intent clearer.
Example refactor direction
class TestResampleSingleState: + `@staticmethod` + def _make_single_state_traj(with_output=False): + class ODE(co.ODESystem): + a = parameter() + x = state() + if with_output: + dynamic_output.velocity = -a * x + dot[x] = -a * x + + class Traj(ODE.TrajectoryAnalysis): + tf = 10 + initial[x] = 1 + if with_output: + vel = dynamic_output.velocity + + return Traj🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_trajectory_analysis.py` around lines 388 - 463, Multiple tests duplicate the same single-state ODE and TrajectoryAnalysis scaffolding (classes named ODE and Traj) — create a small helper to return a ready-to-use trajectory instance and use it in each test instead of repeating the class definitions. Implement a factory function (e.g., make_single_state_traj(a=0.5, tf=10, initial=1, dynamic_output=False)) that builds the ODE/Traj classes (including optional dynamic_output and exposing vel when needed) and returns an instantiated sim; then update test_resample_single_state*, test_resample_single_state_values, test_resample_single_state_no_output, test_resample_single_state_with_dynamic_output, and test_resample_single_state_small_dt to call make_single_state_traj(...) and call sim.resample(...) as before. Ensure the helper preserves parameter names and initial state behavior so existing assertions against resampled.t and resampled.x continue to work.
433-449: Assert dynamic output values, not only successful execution.At Line 447–Line 448, the test proves the call doesn’t crash, but it won’t catch incorrect
dynamic_outputvalues/shaping. Add a value assertion onresampled.velso this path is actually validated.Proposed test strengthening
sim = Traj(a=0.5) resampled = sim.resample(1.0, include_events=False, include_output=True) assert resampled.t.size > 0 + expected_vel = -0.5 * np.exp(-0.5 * resampled.t) + np.testing.assert_allclose(resampled.vel, expected_vel, rtol=1e-4)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_trajectory_analysis.py` around lines 433 - 449, The test currently only ensures resample runs; add an assertion that dynamic_output values are correct by comparing resampled.vel against the analytical velocity -a * exp(-a * t) (or elementwise -a * x where x = exp(-a * t)) from the Traj/Timestep results; use the same a passed to Traj(a=0.5) and compare elementwise across resampled.t (or at the t=1.0 sample) with an approximate-equality check (e.g., allclose/approx) to validate both value and shaping for dynamic_output.velocity in test_resample_single_state_with_dynamic_output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_trajectory_analysis.py`:
- Around line 388-463: Multiple tests duplicate the same single-state ODE and
TrajectoryAnalysis scaffolding (classes named ODE and Traj) — create a small
helper to return a ready-to-use trajectory instance and use it in each test
instead of repeating the class definitions. Implement a factory function (e.g.,
make_single_state_traj(a=0.5, tf=10, initial=1, dynamic_output=False)) that
builds the ODE/Traj classes (including optional dynamic_output and exposing vel
when needed) and returns an instantiated sim; then update
test_resample_single_state*, test_resample_single_state_values,
test_resample_single_state_no_output,
test_resample_single_state_with_dynamic_output, and
test_resample_single_state_small_dt to call make_single_state_traj(...) and call
sim.resample(...) as before. Ensure the helper preserves parameter names and
initial state behavior so existing assertions against resampled.t and
resampled.x continue to work.
- Around line 433-449: The test currently only ensures resample runs; add an
assertion that dynamic_output values are correct by comparing resampled.vel
against the analytical velocity -a * exp(-a * t) (or elementwise -a * x where x
= exp(-a * t)) from the Traj/Timestep results; use the same a passed to
Traj(a=0.5) and compare elementwise across resampled.t (or at the t=1.0 sample)
with an approximate-equality check (e.g., allclose/approx) to validate both
value and shaping for dynamic_output.velocity in
test_resample_single_state_with_dynamic_output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af635ab5-2dd0-474e-a3ff-1d4d07754070
📒 Files selected for processing (1)
tests/test_trajectory_analysis.py
Address CodeRabbit review: assert resampled.velocity values match analytical solution (-0.5 * exp(-0.5 * t)), not just successful execution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_trajectory_analysis.py (1)
388-483: Consider extracting a shared helper/fixture for ODE/Traj setup.The repeated class definitions are very similar across tests; a small fixture would reduce duplication and future maintenance overhead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_trajectory_analysis.py` around lines 388 - 483, Several tests repeat near-identical ODE and Traj classes (e.g., ODE and Traj in test_resample_single_state, test_resample_single_state_values, test_resample_single_state_with_dynamic_output, etc.); extract a small helper or pytest fixture like make_sim(...) that builds and returns the Traj simulator instance (or the Traj class) configured by flags: states (['x'] or ['x','v']), parameter a, tf, initial values, and an optional dynamic_output flag to attach dynamic_output.velocity; replace direct class definitions in each test with calls to make_sim(a=0.5, tf=10, initial={'x':1}, dynamic_output=True/False) so tests reuse the factory and reduce duplication while keeping names ODE and Traj references inside the helper for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_trajectory_analysis.py`:
- Around line 419-431: The test test_resample_single_state_no_output currently
passes include_output=False but the ODE class has no dynamic_output, so the
include_output=False branch in resample isn't exercised; modify the test's ODE
(the ODE class used by Traj) to declare at least one dynamic_output (use the
dynamic_output symbol) so that when you call Traj(...).resample(...,
include_output=False, include_events=False) the code path that checks
model.dynamic_output._count and skips building outputs is actually exercised;
ensure the Traj initial/parameter setup remains the same and keep the assertion
on resampled.t.size.
---
Nitpick comments:
In `@tests/test_trajectory_analysis.py`:
- Around line 388-483: Several tests repeat near-identical ODE and Traj classes
(e.g., ODE and Traj in test_resample_single_state,
test_resample_single_state_values,
test_resample_single_state_with_dynamic_output, etc.); extract a small helper or
pytest fixture like make_sim(...) that builds and returns the Traj simulator
instance (or the Traj class) configured by flags: states (['x'] or ['x','v']),
parameter a, tf, initial values, and an optional dynamic_output flag to attach
dynamic_output.velocity; replace direct class definitions in each test with
calls to make_sim(a=0.5, tf=10, initial={'x':1}, dynamic_output=True/False) so
tests reuse the factory and reduce duplication while keeping names ODE and Traj
references inside the helper for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5f6ded6-114e-4a6c-9980-786dc4878fbd
📒 Files selected for processing (1)
tests/test_trajectory_analysis.py
| def test_resample_single_state_no_output(self): | ||
| class ODE(co.ODESystem): | ||
| a = parameter() | ||
| x = state() | ||
| dot[x] = -a * x | ||
|
|
||
| class Traj(ODE.TrajectoryAnalysis): | ||
| tf = 10 | ||
| initial[x] = 1 | ||
|
|
||
| sim = Traj(a=0.5) | ||
| resampled = sim.resample(1.0, include_events=False, include_output=False) | ||
| assert resampled.t.size > 0 |
There was a problem hiding this comment.
include_output=False path is not truly exercised in this test.
At Line 430 you pass include_output=False, but the ODE in Lines 420-423 has no dynamic_output. In src/condor/contrib.py (Lines 693-750), include_output is gated by model.dynamic_output._count, so this ends up on the same effective path as the basic single-state test.
Suggested adjustment
def test_resample_single_state_no_output(self):
class ODE(co.ODESystem):
a = parameter()
x = state()
+ dynamic_output.velocity = -a * x
dot[x] = -a * x
class Traj(ODE.TrajectoryAnalysis):
tf = 10
initial[x] = 1
sim = Traj(a=0.5)
resampled = sim.resample(1.0, include_events=False, include_output=False)
assert resampled.t.size > 0🧰 Tools
🪛 Ruff (0.15.6)
[error] 421-421: Undefined name parameter
(F821)
[error] 422-422: Undefined name state
(F821)
[error] 423-423: Undefined name dot
(F821)
[error] 427-427: Undefined name initial
(F821)
[error] 427-427: Undefined name x
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_trajectory_analysis.py` around lines 419 - 431, The test
test_resample_single_state_no_output currently passes include_output=False but
the ODE class has no dynamic_output, so the include_output=False branch in
resample isn't exercised; modify the test's ODE (the ODE class used by Traj) to
declare at least one dynamic_output (use the dynamic_output symbol) so that when
you call Traj(...).resample(..., include_output=False, include_events=False) the
code path that checks model.dynamic_output._count and skips building outputs is
actually exercised; ensure the Traj initial/parameter setup remains the same and
keep the assertion on resampled.t.size.
|
upstream nasa#75 closed by maintainer (2026-04-12) |
Summary
Fix
TrajectoryAnalysis.resamplefailing for ODESystems with a single state variable.Fixes nasa#70
Summary by CodeRabbit
Bug Fixes
Tests